LK R&D LKSM - Update default audit level #6945
Conversation
| { | ||
| void addSummaryAuditEvent(User user, Container c, TableInfo table, QueryService.AuditAction action, Integer dataRowCount, @Nullable AuditBehaviorType auditBehaviorType, @Nullable String userComment); | ||
|
|
||
| default void addSummaryAuditEvent(User user, Container c, TableInfo table, QueryService.AuditAction action, Integer dataRowCount, @Nullable AuditBehaviorType auditBehaviorType, @Nullable String userComment, boolean skipAuditLevelCheck) |
There was a problem hiding this comment.
There doesn't seem to be any actual usage of the skipAuditLevelCheck. Is this something that can be removed?
There was a problem hiding this comment.
It is used to bypass check for ETL in bulkLoad mode and some other places, it's overriden inAbstractAuditHandler.
|
|
||
| @NotNull | ||
| @Override | ||
| public AuditBehaviorType getDefaultAuditBehavior() |
There was a problem hiding this comment.
This override looks like it just repeats what TableInfo already specifies. Consider removing.
There was a problem hiding this comment.
Good catch, it actually did mean to override. Fixed.
| setAuditBehavior(auditBehaviorType); | ||
| } | ||
| catch (IllegalArgumentException ignore) | ||
| { |
There was a problem hiding this comment.
nit: Consider logging a warning about an invalid audit behavior argument here. Same for similar usage in SchemaTableInfo.
| { | ||
| for (Long runId: dataChangeCount.keySet()) | ||
| { | ||
| String userComment = configParameters == null ? null : (String) configParameters.get(AuditUserComment); |
There was a problem hiding this comment.
This userComment processing can be moved above the loop.
| Map<String, Object> extraScriptContext | ||
| ) throws InvalidKeyException, BatchValidationException, QueryUpdateServiceException, SQLException | ||
| { | ||
| dataChangeCount = new HashMap<>(); |
There was a problem hiding this comment.
Consider using a LinkedHashMap to retain insertion order so that audit events are consistently added in the same order.
| return result; | ||
| } | ||
|
|
||
| private Object lookupDisplayValue(Object o, @NotNull TableInfo fkTableInfo, ColumnInfo fkTablePkCol) |
There was a problem hiding this comment.
Really nice to see this go away.
| @Override | ||
| public AuditBehaviorType getAuditBehavior() | ||
| @NotNull | ||
| public AuditBehaviorType getDefaultAuditBehavior() |
There was a problem hiding this comment.
Should ExpDataClassTableImpl also override this and set to DETAILED?
There was a problem hiding this comment.
I don't think so, ExpDataClassTableImpl is for dataclass type not the data itself. For domains, we have DomainEvent and DomainPropertyEvent.
| } | ||
| else if (tInfo != null) | ||
| { | ||
| ExpSampleType sampleType = SampleTypeService.get().getSampleType(c, tInfo.getUserSchema().getUser(), tInfo.getName()); |
There was a problem hiding this comment.
nit: No need to call SampleTypeService.get() as this call is being made from within the service itself.
| // NOTE: to avoid a diff in the audit log make sure row("rowid") is correct! (not the unused generated value) | ||
| row.put(ROW_ID,staticsRow.get(ROW_ID)); | ||
| } | ||
| else if (tInfo != null) |
There was a problem hiding this comment.
tInfo.getUserSchema() could be null. Consider:
else if (tInfo != null)
{
UserSchema schema = tInfo.getUserSchema();
if (schema != null)
{
ExpSampleType sampleType = getSampleType(c, schema.getUser(), tInfo.getName());
if (sampleType != null)
{
event.setSampleType(sampleType.getName());
event.setSampleTypeId(sampleType.getRowId());
}
}
}
2 participants
Rationale
Currently, the logic to determine the effective audit behavior is: XML metadata override > API param > table’s default setting (which currently is NONE). With this story, the logic is now updated to Max(xml??tableDefault, api)’s priority. Additionally, the default behavior for samples, dataclass data and assay results are updated to DETAILED. Note that for assay results, the detailed audit logging currently only applies for update/delete actions done using QUS.
Related Pull Requests
Changes